-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a button to create a new note to the dashboard #873
Add a button to create a new note to the dashboard #873
Conversation
Signed-off-by: Kévin Cocchi <kevin.cocchi@gmail.com>
Signed-off-by: Kévin Cocchi <kevin.cocchi@gmail.com>
Signed-off-by: Kévin Cocchi <kevin.cocchi@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, @salixor ! This is a good solution. However, I have some questions to the code changes (see inline comments).
src/App.vue
Outdated
this.routeToNote(note.id, { new: null }) | ||
this.routeToNote(note.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you remove the parameter new
? It has the purpose, that the note's title is automatically generated from the first line of the note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I hadn't seen it was used somewhere else ! I'll remove these changes
src/components/Dashboard.vue
Outdated
buttonsFooterStyle() { | ||
const marginTop = this.items.length > 0 ? 20 + (this.displayedItemsCount - this.items.length) * 60 : 10 | ||
return { marginTop: `${marginTop}px` } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks quite complicated. You are comparing to the tasks app, but I don't see such a complicated calculation, there. Do we really need this or can we copy the styling from the tasks app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main motive behind this implementation was to have the button stay at the bottom of the widget, rather than below the content. In Tasks, the button stays below the content :
I'd argue having the button at a fixed height is better, but we could as well simplify the logic and have the button stay below the content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then we should do absolute positioning from the bottom. I will provide a change suggestion in a minute.
Signed-off-by: Kévin Cocchi <kevin.cocchi@gmail.com>
Signed-off-by: Kévin Cocchi <kevin.cocchi@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to do absolute positioning instead of calculating the position from top. This also obsoletes displayedItemsCount
. What do you think?
src/components/Dashboard.vue
Outdated
buttonsFooterStyle() { | ||
const marginTop = this.items.length > 0 ? 20 + (this.displayedItemsCount - this.items.length) * 60 : 10 | ||
return { marginTop: `${marginTop}px` } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then we should do absolute positioning from the bottom. I will provide a change suggestion in a minute.
Signed-off-by: Kévin Cocchi <kevin.cocchi@gmail.com>
I had tried it, but I didn't reach a satisfying solution. I'm now realizing I simply had put the Thanks for the suggestion, it's definitely cleaner this way 👍 I also replaced the translation string to prevent the need of translating to every language, since there's already a string that does the job well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good and sorry for the delay
Closes #869 by adding a new button :
For reference, next to the Tasks app widget :